test: assert cat_tools' schema(s) are never on search_path by jnasbyupgrade · Pull Request #76 · Postgres-Extensions/cat_tools

jnasbyupgrade · 2026-08-03T21:38:39Z

Add a permanent, real pgTAP test that asserts cat_tools/_cat_tools are never part of the resolved search_path during testing. test/finish.sql is \i'd by every test/sql/*.sql file right before finish(), checking current_schemas(false) -- Postgres's own unqualified-name resolution list. It's a real, plan()-counted assertion rather than a silent DO block, so a future test file bumping its own plan() count has to notice it exists.

Checking at the end of each file, not just at setup, is what matters: pgxntool's own tap_setup.sql already keeps search_path clean at the start of every test file, so a start-only check can only prove that much -- it can't see a test that mutates search_path partway through and leaves it that way. Checking again right before finish() catches that case too. One caveat: a test that mutates search_path and then restores it before this check runs would still slip through.

Test plan

make verify-results passes locally with the new test/finish.sql in place, and test/expected/*.out regenerated via make results
Manually confirmed the guard fires: injected a mid-file SET search_path = cat_tools, tap, public into a scratch test file and confirmed only that file's test/finish.sql check failed -- files run afterward were unaffected, confirming the per-file connection/rollback isolation
make lint clean
CI green on this PR

… search_path cat_tools' control file pins schema = 'cat_tools' with relocatable = false -- it can never be installed into a user-chosen schema, so there was never a "which schema" question to test. The real, narrower risk is that some cat_tools view/function could call another cat_tools object unqualified, relying on search_path to resolve it -- which would keep working by accident in any session that happens to have 'cat_tools' searchable, and only break for a real caller who doesn't. Verified empirically before adding this: pgxntool's own tap_setup.sql (\i'd by every test/sql/*.sql file via test/setup.sql) already sets `search_path = tap, public` -- cat_tools' schemas are already never on it today, for every test in the suite, with no toggle needed. This assertion just makes that fact explicit and self-documenting instead of true by accident of the shared baseline, so a future change to it fails loudly instead of silently widening what "passing" means. current_schemas(false) is Postgres's own unqualified-name search list, so checking cat_tools/_cat_tools are absent from it is a direct proof, not a proxy for one (confirmed the check fires: manually SET search_path to include 'cat_tools' and confirmed the RAISE EXCEPTION triggers with the expected message). Always-on, no GUC, no variable, no separate test target -- this is not a toggleable dimension, just a permanent fact this suite now checks.

coderabbitai · 2026-08-03T21:38:47Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ae1dc61f-9d33-47a9-8d32-ac4e8522849c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-08-03T21:44:22Z

Claude finished @jnasbyupgrade's task in 1m 56s —— View job

Code review

Summary: Adds a DO $$ ... $$ guard to test/setup.sql (23 lines, additions only) that raises an exception if cat_tools or _cat_tools appear in current_schemas(false) — asserting cat_tools' own schemas are never resolvable via unqualified search_path during tests.

Checks performed:

Confirmed __cat_tools (the third, double-underscore schema created during install) is dropped at the end of the install script (sql/cat_tools.sql.in line 1981, DROP SCHEMA __cat_tools;), so it never exists post-install and correctly isn't part of the assertion — only cat_tools and _cat_tools persist and need checking.
current_schemas(false) excludes implicit schemas (pg_catalog, temp schema) and returns only what's explicitly on search_path, matching the PR's stated intent (catching unqualified cross-references, not implicit resolution).
Comment uses the required /* ... */ block format for the multi-line explanation (per CLAUDE.md's SQL comment convention).
The DO $$ ... $$ block has no nested dollar-quoted/non-indentable blocks inside it (just a single IF ... END IF, which indents normally), so CLAUDE.md's "label the closer" rule for non-indentable blocks containing nested blocks doesn't apply here — no ambiguity in what $$; closes.
Placement (after \i test/pgxntool/setup.sql, which sets search_path = tap, public, and before the rest of setup.sql) is correct for the check to be meaningful.

No issues found. Checked for bugs and CLAUDE.md compliance.

…ile end Replace test/setup.sql's DO-block assertion (checked once, at the START of every test file) with test/finish.sql, a real, plan()-counted pgTAP test \i'd right before finish() at the END of every test/sql/*.sql file. This is strictly stronger: a start-only check cannot see a test that mutates search_path partway through a file. Confirmed empirically that each test/sql file runs as its own psql connection wrapped in a transaction that never commits (rolled back when the connection closes), so a plain SET search_path already cannot leak into a LATER file regardless of when it's checked -- the real gap a start-only check leaves open is a mutation that stays in effect for the rest of THAT SAME file's own tests, which checking again at the end now catches. Not foolproof: a test that mutates search_path and then restores it before this check would still slip through. Making it a counted pgTAP test (not a DO block) means a new test file author bumping their own plan() count has to notice this test exists -- a natural reminder not to add a search_path-dependent test without thinking about it. Every test/sql/*.sql file's plan() count is bumped by 1 and expected output regenerated via `make results` accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

… second \i Each test/sql/*.sql file now has exactly one finish-side \i (test/finish.sql), mirroring how they already have exactly one setup-side \i (test/setup.sql, which itself \i's test/pgxntool/setup.sql as its first line). test/finish.sql runs the search_path assertion and then \i's test/pgxntool/finish.sql as its last line, rather than every test file calling both directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions · 2026-08-03T22:38:14Z

Claude finished @jnasbyupgrade's task in 1m 39s —— View job

Code review

Summary: Replaces the earlier DO $$ ... $$ guard in test/setup.sql with a real, counted pgTAP assertion in a new test/finish.sql, \i'd by every test/sql/*.sql file (13 files) in place of the direct \i test/pgxntool/finish.sql, immediately before pgxntool's own finish(). test/setup.sql now only carries an explanatory comment (no assertion) pointing at the new location. test/finish.sql asserts cat_tools/_cat_tools are absent from current_schemas(false), then chains to test/pgxntool/finish.sql. All 13 test/sql/*.sql plan() counts are bumped by +1 and all 13 test/expected/*.out files are regenerated to match.

Checks performed:

Verified all 13 files under test/sql/*.sql were updated consistently: \i test/pgxntool/finish.sql → \i test/finish.sql, plan count +1 with a -- search_path still clean (test/finish.sql) comment, no file missed.
Cross-checked each test/expected/*.out diff against its corresponding .sql change — plan count and new ok N - cat_tools schema(s) must not be part of the resolved search_path -- got {tap,public} line match in every case (spot-checked attribute.out, general.out, pg_depends.out).
test/sql/pg_depends.sql runs SET LOCAL ROLE :"use_role" immediately before \i test/finish.sql; the expected output confirms the new assertion still passes under that role — i.e. a role with USAGE granted on cat_tools/_cat_tools schemas still doesn't have them on search_path, which is the correct distinction the test is making (grants ≠ search_path membership).
test/finish.sql's comment is in the required /* ... */ block format; test/setup.sql's replacement explanatory comment is too. No -- used for multi-line explanations.
Confirmed there's no non-indentable nested block in test/finish.sql needing a labeled closer (just a single SELECT ok(...) followed by an \i) — CLAUDE.md's closer-labeling rule doesn't apply.
Logic of the guard itself is sound: NOT ('cat_tools' = ANY(current_schemas(false)) OR '_cat_tools' = ANY(current_schemas(false))) correctly checks Postgres's own unqualified-name resolution list, and chaining to test/pgxntool/finish.sql (rather than duplicating a second \i) matches the pattern test/setup.sql already uses for test/pgxntool/setup.sql.
PR body's rationale for checking at file-end rather than file-start (catching search_path mutations that persist for the rest of that file, given each test/sql/*.sql runs in its own rolled-back transaction) is consistent with pgxntool's documented per-file connection/rollback behavior.

No issues found.

The previous comment recounted the empirical investigation that justified this design (psql-connection-per-file, transaction rollback semantics) -- that belongs in the PR description, not as a permanent code comment. Keep only the conclusion: what this asserts, why file-end (not just setup), the one caveat, and the plan()-bump reminder. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions · 2026-08-03T22:52:38Z

Claude finished @jnasbyupgrade's task in 2m 34s —— View job

Code review

Summary: Since the last review pass (which covered through 6e7a82a), the only change is effbfbc, which trims test/finish.sql's block comment down to its conclusion (what's asserted, why file-end, the one caveat, the plan()-bump reminder) and drops the empirical-investigation narrative, per the PR body's own stated rationale that this belongs in the PR description, not a permanent comment.

Finding

test/finish.sql:17-18 — the two-line -- Chain through to pgxntool's own finish, same wrapping pattern as test/setup.sql / does for test/pgxntool/setup.sql. comment is one explanation split across two -- lines. CLAUDE.md's SQL comment convention is explicit: "Always use block comment format for multi-line comments in SQL files... Never use -- line comments for multi-line explanations." This predates effbfbc (present since 6e7a82a) but is still in the file as of HEAD and wasn't flagged in the earlier passes. Minor, easy fix: fold it into a /* ... */ block (either standalone or merged into the block comment just above it).

Fix this →

Everything else checked and unchanged from prior passes: the shortened comment's content is accurate and still explains the "why file-end" rationale and the mutate-then-restore caveat without recounting the empirical investigation history (matches CLAUDE.md's Bug Fixes guidance — no novels, no unnecessary history). Logic of the ok() assertion, the chain to test/pgxntool/finish.sql, and all 13 test/sql/*.sql / test/expected/*.out pairs were already verified consistent in the prior review pass and are untouched by this commit.

No other issues found.

…bump aside Convert the trailing two-line -- comment to a /* */ block comment per CLAUDE.md's Code Style section (never -- for multi-line explanations). Also drop the "bump that file's plan() by one" clause from the main comment -- anyone writing a test already knows to keep plan() in sync with its assertions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions · 2026-08-04T18:08:56Z

Claude finished @jnasbyupgrade's task in 58s —— View job

Code review

Gather context (PR diff, CLAUDE.md files)
Summarize the change since last review
Review for CLAUDE.md compliance
Review for bugs/logic issues
Validate flagged issues
Post findings

Summary: Since the last review pass (which covered through effbfbc), the only change is a38750b, which:

Converts the two-line -- comment at the bottom of test/finish.sql (explaining the chain to test/pgxntool/finish.sql) into a proper /* ... */ block comment.
Drops the "bump that file's plan() by one" clause from the main comment at the top of the file.

Checks performed:

This is exactly the fix requested in the prior review pass — the CLAUDE.md violation (-- used for a multi-line explanation, forbidden by the Code Style section) is resolved. test/finish.sql now uses /* ... */ for both comment blocks.
Confirmed no other file in the PR changed in this commit (git diff cdfa968^..HEAD --stat matches exactly what was already verified consistent in prior passes: 13 test/sql/*.sql + 13 test/expected/*.out pairs, test/setup.sql, test/finish.sql).
Dropping the plan()-bump reminder is a reasonable trim per CLAUDE.md's Bug Fixes guidance ("be concise... say the point in as few words as it needs") — a test author adding an assertion to a file already has to update that file's plan() count regardless, so the reminder was redundant.
Re-verified the surviving comment content: still explains what's asserted, why file-end (not just setup), and the one caveat (mutate-then-restore slips through) — accurate and matches the PR body's stated rationale.

No issues found.

jnasbyupgrade and others added 2 commits August 3, 2026 17:29

jnasbyupgrade changed the title ~~test/setup.sql: assert cat_tools' own schemas are never on search_path~~ test: replace search_path assertion with a real pgTAP test, checked at file end Aug 3, 2026

jnasbyupgrade changed the title ~~test: replace search_path assertion with a real pgTAP test, checked at file end~~ test: assert cat_tools' schema(s) are never on search_path Aug 4, 2026

Navigation Menu

Uh oh!

test: assert cat_tools' schema(s) are never on search_path - #76

test: assert cat_tools' schema(s) are never on search_path#76
jnasbyupgrade wants to merge 5 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:search-path-assertion

jnasbyupgrade commented Aug 3, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 3, 2026 •

edited

Loading

Review skipped

Uh oh!

github-actions Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jnasbyupgrade commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

coderabbitai Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review

Uh oh!

github-actions Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review

Uh oh!

github-actions Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review

Uh oh!

github-actions Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jnasbyupgrade commented Aug 3, 2026 •

edited

Loading

coderabbitai Bot commented Aug 3, 2026 •

edited

Loading

github-actions Bot commented Aug 3, 2026 •

edited

Loading

github-actions Bot commented Aug 3, 2026 •

edited

Loading

github-actions Bot commented Aug 3, 2026 •

edited

Loading

github-actions Bot commented Aug 4, 2026 •

edited

Loading